Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update byte_format.cr example to not use uninitialized #15128

Closed
wants to merge 1 commit into from

Conversation

BigBoyBarney
Copy link

Using an unsafe method as the primary example isn't particularly good IMO. Initialising the StaticArray with 0s comes with a slight overheard, but I still think it's a better example use-case.

@straight-shoota
Copy link
Member

Even taking a reference to a stack-allocated StaticArray is potentially unsafe because it could leak stack pointers. So bytes.to_slice is unsafe in a similar way as uninitalized is (ref #9606). Removing one but keeping the other seems unreasonable.
If we want the code sample to be free of unsafe code, we should use a heap-allocated Slice instead of StaticArray.

uninitialized is the typical way to define a StaticArray because it's used for performance optimization or C bindings where you need to care about the details anyway. And I'd argue that IO::ByteFormat has similar use cases, so it seems natural to demonstrate the usage with uninitalized StaticArray.

StaticArray.new on the other hand is not a realistic example because this method is only used when you explicitly need to fill the array with a specific value, which is not the case when you pass it to a IO::ByteFormat.encode method.

@BigBoyBarney
Copy link
Author

Interesting, I did not know it worked like that.

One could also argue that initialising a StaticArray with 0s just to overwrite it on the next line if wasteful.

How about leaving the current example in, labelling it as unsafe, and adding another, safe example using Slice?

# Unsafe due to the uninitialized `StaticArray`. The buffer is allocated on the stack, avoiding a heap allocation.
raw = uninitialized UInt8[2]
IO::ByteFormat::LittleEndian.encode(0x1234_i16, raw.to_slice)
raw # => StaticArray[0x34, 0x12]

# Safe, allocated on the heap.
slice = Bytes.new 2
IO::ByteFormat::LittleEndian.encode(0x1234_i16, slice)
slice # => Bytes[0x34, 0x12]

@straight-shoota
Copy link
Member

straight-shoota commented Oct 25, 2024

Sounds good.
I wouldn't call the current code example "unsafe" though. Some of the features it uses are (potentially) unsafe, but the way they are used in this example is entirely safe.

@ysbaddaden
Copy link
Contributor

Note: the GC even guarantees that the memory has been initialized to zero.

For a larger buffer, like reading from an IO, we might have wanted to explain that it's fine to not initialize the buffer because we will overwrite it anyway.

But a 2 bytes buffer? That's not worth it. In fact, the LLVM optimizer will notice that we overwrite the whole memory and only then try to read from it, so the safe initialization can be safely skipped.

I compiled a program with that snippet, one function with the safe initializer, and another function with uninitialized: both are compiled to the exact same assembly; in fact LLVM even precomputed the actual result of the byte format and it hardcoded the actual values 🤷

mov    $0x34,%edi
mov    $0x12,%esi

@BigBoyBarney
Copy link
Author

Fascinating!

What would happen if the bytes to decode were known only at runtime though?

In any case, I suppose the docs can be left as they are, as the uninitialized here isn't as bad as I initially thought :D Thanks for your inputs!

@ysbaddaden
Copy link
Contributor

@BigBoyBarney then it wouldn't optimize the byte format call, but it can still detect that the memory is fully written to, and skip the initialization to zero

@BigBoyBarney
Copy link
Author

Would it be allocated on the heap or the stack?
StaticArray → stack
Slice / Bytes → heap normally, would this be optimised to the stack too? (in the case of newing up a Slice with size 2)

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Oct 26, 2024

No. LLVM has no control over where memory is allocated.

There's a RFC 4 to get crystal to do this optimization.

@BigBoyBarney
Copy link
Author

Very interesting. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants